Add virtual_address_update disable #249
Conversation
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
services/crypto_update.proto
Outdated
| bytes remove = 21; | ||
| * The 20-byte EVM address of the virtual address that is being disabled and therefore removed from active use on the account. | ||
| */ | ||
| bytes disable = 21; |
There was a problem hiding this comment.
Since a disabled address stays in state, shouldn't this flag be moved to VirtualAddress? If queries only return active addresses how would users know which old, inactive addresses they need to delete?
There was a problem hiding this comment.
Yeah good suggestion. Will leave till we're ready to turn on disable
services/crypto_update.proto
Outdated
| * The 20-byte EVM address of the virtual address that is being removed. | ||
| */ | ||
| bytes remove = 21; | ||
| * The 20-byte EVM address of the virtual address that is being disabled and therefore removed from active use on the account. |
There was a problem hiding this comment.
The only difference between disable and remove is that the former retains ownership and the latter allows reuse. Add a comment to each field about the impacts of address reuse.
There was a problem hiding this comment.
Will do when we circle back
services/crypto_update.proto
Outdated
| bytes remove = 21; | ||
| * The 20-byte EVM address of the virtual address that is being disabled and therefore removed from active use on the account. | ||
| */ | ||
| bytes disable = 21; |
There was a problem hiding this comment.
Don't think we should add things to proto that won't be implemented in the next release. We tend to do that and never implement it or want to implement it differently.
There was a problem hiding this comment.
That's fair.
We'll coordinate the disable addition.
Will remove as suggested since adding to a oneof later on (even if the ordinal value is greater than existing outer fields) won't break the API
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
services/crypto_update.proto
Outdated
| @@ -167,17 +167,8 @@ message CryptoUpdateTransactionBody { | |||
| oneof virtual_address_update { | |||
| /** | |||
| * The virtual address to be added. | |||
| * The address one successfully addition becomes par tof the accounts ACTIVE virtual addresses | |||
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
services/crypto_update.proto
Outdated
| @@ -167,7 +167,7 @@ message CryptoUpdateTransactionBody { | |||
| oneof virtual_address_update { | |||
| /** | |||
| * The virtual address to be added. | |||
| * The address one successfully addition becomes par tof the accounts ACTIVE virtual addresses | |||
| * The address one successfully addition becomes part of the accounts ACTIVE virtual addresses | |||
There was a problem hiding this comment.
Once successfully added, the address becomes part of the account's ACTIVE virtual addresses.
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Nana-EC
requested review from
steven-sheehy,
kimbor,
tinker-michaelj and
Neeharika-Sompalli
Yes, will move this into draft. |
|
So what's here now is removing the field |
|
Closing until HIP 631 is revisited with HIP 583 induced changes |
Signed-off-by: Nana Essilfie-Conduah [email protected]
Description:
To support the ability to disable a virtual address but keep it associated with an account
disabletoCryptoUpdateTransactionBody.virtual_address_updateRelated issue(s):
Fixes #248
Notes for reviewer:
Checklist